-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use the same regex as used in ExpensiMark #19216
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@neil-marcellini @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well, but now I'm thinking we should take a slightly different approach for consistency.
src/libs/actions/Report.js
Outdated
const regex = new RegExp( | ||
`\\[(?:[^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`, | ||
'gm', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have mentioned this when reviewing your proposal, but I think it would actually be better to import the exact RegExp from expensify common so that we don't have any code duplication.
Already you can see that we have changed this in expensify common, so if we import the exact RegExp from there then they will always be in sync. So we'll need to make a change in expensify-common too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do I need to create a PR in Expensify-common as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the need of ?:
in this PR.
@s-alves10 can you elaborate more why this is needed? Take example cases.
If we apply this in expensify-common, it causes many regressions as automated test says.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aimane-chnaif @neil-marcellini
Let me explain. We uses match[1] to extract URLs here
App/src/libs/actions/Report.js
Lines 908 to 910 in 2182d0c
// Element 1 from match is the regex group if it exists which contains the link URLs | |
const links = _.map(matches, (match) => Str.sanitizeURL(match[1])); | |
return links; |
?:
means non-capture group. If we don't use it, we may need to use match[2]
In order to sync with expensify-common
as you mentioned above, I need to create a PR in expensify-common
(need to add named export for link regex) and use match[2]
without ?:
Is this what you want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the same regex as expensify-common. I think it would be better to use match[2]
. Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but still need to raise PR in expensify-common for reusing regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the same regex as expensify-common. I think it would be better to use
match[2]
. Do you agree?
We should fix </pre>
case as it doesn't happen on main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the latest changes of expensify-common
isn't used in App yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aimane-chnaif doesn't work? How can I reproduce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-alves10 #18911 (comment)
This should be regression because it doesn't happen on main
update to latest change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I'll let @aimane-chnaif test too and make sure it works well.
Screen.Recording.2023-05-23.at.11.41.33.AM.mov
Reviewer Checklist
Screenshots/VideosWebweb.movDesktopdesktop.mov |
I also confirmed that this PR magically fixes autolink issue in edited message. Before: Screen.Recording.2023-05-23.at.8.04.29.PM.movAfter: Screen.Recording.2023-05-23.at.8.05.48.PM.mov |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.18-0 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.18-0 🚀
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.3.18-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.18-2 🚀
|
Details
Fixed Issues
$ #18911
PROPOSAL: #18911 (comment)
Tests
[reference](google.com)
)[[reference]](google.com)
)Offline tests
[reference](google.com)
)[[reference]](google.com)
)QA Steps
[reference](google.com)
)[[reference]](google.com)
)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
mac_chrome_18911.mp4
Mobile Web - Chrome
android_chrome_18911.mp4
Mobile Web - Safari
ios_safari_18911.mp4
Desktop
mac_desktop_18911.mp4
iOS
ios_18911.mp4
Android
android_18911.mp4